Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib.path.removePrefix: init #238013

Merged
merged 1 commit into from
Jul 19, 2023
Merged

lib.path.removePrefix: init #238013

merged 1 commit into from
Jul 19, 2023

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Jun 15, 2023

Description of changes

Adds a new function to lib.path:

  • lib.path.removePrefix: Remove the first path as a component-wise prefix from the second path.
    removePrefix /foo /foo/bar/baz
    -> "./bar/baz"

Originally split off from #210423, but changed after reconsideration here. Part of the path library effort.

Depends on #237610 now merged

This work is sponsored by Antithesis

Things done
  • docs
  • tests

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jun 16, 2023
Comment on lines 175 to 233
- `success` (boolean): Whether the first path is a component-wise prefix of the second path.
This is the same as `lib.path.hasPrefix`.
If this is `false`, the other attributes will throw an error when evaluated.

- `components` (list of strings): The resulting path components after removing the prefix.
If the first path is the same as the second path, this value is `[ ]`.

- `subpath` (string): The result as a normalised subpath string, see `lib.path.subpath.normalise`
This is the same as `lib.path.subpath.join components`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clever but cumbersome, and not more convenient than

if hasPrefix a b
  then subpath.join (removePrefix a b)
  else throw "oopsie"

We can still keep the good error messages in case people don't bother to do the check on their own.

If you're worried about consistency with passing around components instead of a subpath string – well, there is the type signature and one can always add a doc comment stating this is more efficient and one can easily bake it back together into a string with subpath.join.

An alternative is providing a "convenience" layer such as removePrefixAsSubpath (giving the one doing more work a longer name), but this is more to type than the explicit join.

Copy link
Member Author

@infinisil infinisil Jun 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that it's not clear what removePrefix should return. If it returns a subpath string but you need coomponents, you'd have to parse them (I want to add subpath.components for that in the future), but that operation is a bit expensive.

In turn if removePrefix were to return components, it would be inconsistent because append doesn't take components, but rather a subpath.

To make it obvious and unambiguous what the function returns and have both use cases, we could do removePrefixAsSubpath and removePrefixAsComponents, but at that point an API as suggested in this PR feels cleaner.

For more context, I would also like a function like this in the future:

lib.path.parts :: Path -> {
                    root :: FilesystemRoot;
                    components :: [ Component ];
                    subpath :: Subpath;
                  }

Which would have a very similar interface, and where alternatives wouldn't be as great.

Maybe that function should come before this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lib.path.parts :: Path -> {
                root :: FilesystemRoot;
                components :: [ Component ];
                subpath :: Subpath;
              }

This is what's currently called deconstructPath somewhere internally, right?

There could be two sets of APIs, one for component-wise paths and one for strings. We also originally discussed a structured representation that is string-coercible, but you deemed that too expensive performance-wise.

I'm personally inclined to keep the interface and implementation simple even at the potential cost of performance at scale. We have nice primitives that would make additions almost trivial, and I think we should use them. If that indeed becomes a real performance problem, one can still go measure and figure out alternatives. For now I'd prefer handling strings as we started out with. It's also more predictable for the user, as at each step you'd get out what you expect a path to look like. Otherwise we're opening up the can of design worms all over again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that it's not clear what removePrefix should return.

I think I'd expect a subpath because these functions have string counterparts.

I agree with @fricklerhandwerk that it's ok to have more functions.

potential cost of performance

I was going to say that __toString is cheaper than outPath, but I guess we're dealing with a list 🤡
Just an Attr of 24 bytes added to an existing contiguous array and no extra thunk if __toString doesn't have the value in its scope, but rather uses the argument. iirc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I now changed this function to return a subpath string. I also created #242695 to get the components of a subpath.

@infinisil infinisil marked this pull request as ready for review June 21, 2023 15:23
@infinisil infinisil requested a review from edolstra as a code owner June 21, 2023 15:23
@infinisil infinisil force-pushed the lib.path.removePrefix branch from 9a7dfac to 1a8329d Compare June 21, 2023 15:25
@roberth roberth added the 6.topic: lib The Nixpkgs function library label Jul 8, 2023
@infinisil infinisil force-pushed the lib.path.removePrefix branch from 1a8329d to 6626d8c Compare July 10, 2023 19:26
@infinisil infinisil mentioned this pull request Jul 19, 2023
7 tasks
@roberth roberth merged commit 814f067 into NixOS:master Jul 19, 2023
@infinisil infinisil deleted the lib.path.removePrefix branch July 19, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants